Stef -- Carets by SesameSeeds · Pull Request #32 · Ada-C8/ada-trader

SesameSeeds · 2017-12-18T10:06:18Z

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question	Answer
How do Backbone Views help structure your code?	Organization of the where and what. However, it's a bit confusing with so many and understanding the best places to put things. I felt there was a lot of trail and error.
Did you use jQuery directly in your Views? How, and why?	Yes, here and there, such as; this.$('css.selector') and $el.
What was an example of an event you triggered? Why did you need to trigger it?	Click functions for buy/sell so the appropriate things can be listened for and return the appropriate bought/sold stock.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different?	No idea if mine pass or not because of time, but I took the tests available in quote and made one for orders and wrote some out. The layout is similar to Ruby, you still do the same describing of what you are testing, testing for, and the three a's. Syntax is different, which is expected in a new language.

… involved the first set of user stories.

…/selling with cost.

…cess.

SesameSeeds · 2017-12-18T10:07:20Z

WIP, still not working properly with trade costs, ran out of time, will fix this asap.

…at I need to do will add asap.

CheezItMan · 2017-12-18T21:52:22Z

Ada Trader

What We're Looking For

Feature	Feedback
Core Requirements
Git hygiene	Decent number of commit and good commit messages, but a lot were added this weekend, what's keeping things from getting done in the week?
Comprehension questions	Using `this.$` doesn't use jQuery directly, instead it uses a jQuery selection within the view. This prevents you from causing a problem outside your view's area of concern. And no your tests don't pass.
Organization
Models and collections are defined in separate files	Check
Code that relies on the DOM is located in or called by `$(document).ready`	Check
Functionality
Quote prices change when clicking Buy and Sell	Check
The Trade History updates when buying and selling a quote	Check
A user can create an open order using the Order Entry Form	Check, but only for `buy` `sell` doesn't work because some of the placeholder values you put in for buy don't exist in sell.
An open order removes itself from the open orders and updates the Trade History when fulfilled	The order isn't listening for changes on the `Quote` model, so it isn't done yet. I assume you ran out of time.
General
Has separate views for different parts of the app	Check
Uses events (listening/handling and triggering) to manage different behavior in views	It works for changes in Quotes updating the Quote view and additional orders updating OrderListView, but changes in quote models don't trigger a response in Order.
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns `this` in render)	note in `quote_view.js` line 29, you're using jQuery instead of `this.$(`. That's a no-no.
Error handling for the Order Entry Form	You have a start on it with validations in `Order`, but currently it's not being used.
Testing
Has unit tests for models	Some tests are written, they aren't passing.
Overall	You did a pretty good job getting Wave 1 & 2 done, there are issues, as I've documented in your code, but they work and you have the general structure working. Wave 3 is where it comes apart. You need to study how backbone objects can communicate together with events. That's clearly where you need to review. Consider passing the `QuoteList` into the `OrderListView` and using it to create orders who then know what quote to watch, and can listen to `change` events. This resource may help a bit. You can also check the textbook curriculum or myself or Charles or Dee.

CheezItMan · 2017-12-18T21:27:28Z

src/views/order_list_view.js

+      buy: true,
+      symbol: this.$('select[name=symbol]').val(),
+      targetPrice: parseFloat(this.$('input[name=price-target]').val()),
+      currentPrice: 0,


How is the order to get the current quote price? Maybe this view needs to have an instance variable referring to the Quotelist.

CheezItMan · 2017-12-18T21:28:12Z

spec/models/order_spec.js

+        targetPrice: 50.00,
+      });
+
+      expect(order.isValid()).toEqual(true);


Validation failing

CheezItMan · 2017-12-18T21:28:41Z

spec/models/order_spec.js

+        action: true,
+      });
+
+      expect(order.isValid()).toEqual(false);


Validation failing

CheezItMan · 2017-12-18T21:32:56Z

src/models/order.js

+    else if (!this.buy && this.targetPrice <= this.currentPrice) {
+      error.targetPrice = ['Price is lower than market, aim higher!'];
+    }
+    else if (!attributes.targetPrice) {


You also need (based on your tests) to check for numeric value for price, boolean value for buy and maybe check if (!attributes.targetPrice) first.

CheezItMan · 2017-12-18T21:38:17Z

src/views/order_list_view.js

+    const orderData = {
+      buy: false,
+      symbol: this.$('select[name=symbol]').val(),
+      targetPrice: parseFloat(this.$('input[name=price-target]').val()),


No currentPrice

CheezItMan · 2017-12-18T21:40:31Z

src/views/quote_view.js

+  sellQuote: function() {
+    this.model.set('buy', false);
+   let tradeTemplate = _.template($('#trade-template').html());
+   $('#trades').prepend(tradeTemplate(this.model.attributes));


You should not directly access jQuery in a view. Instead use this.$ to select things within the view. Since the trades section is not within the view you can can trigger an event and let the larger view handle the prepending.

SesameSeeds added 19 commits December 11, 2017 16:37

View files added.

62f9465

Can view quotes, with symbols and prices and all that good stuff that…

6701682

… involved the first set of user stories.

Fixed bug with adding and subtracting with buy and sell.

3c91903

Can buy a quote.

3d71ed7

Removed unnecessary code.

11a4a3b

Can sell quotes, both buying and selling meets requirements.

5183778

Added files for orders in views and models.

4958383

Addition of necessary import/export information.

d0b1d9d

Collections order_list added.

79fbbb4

Added orderList and orderListView to app.js, similar logic to quote.

249a504

Added basic necessities to build on logic, initially built off quotes.

d6cfc70

Events added for buying/selling orders, working on buy order logic.

570d60f

Buy logic WIP.

95d7cc7

Sell order logic added, working on getting it to work properly.

ce83f10

Added destroy for cancel button.

4359396

Form working, can buy/sell with no price.

86caafb

Mild clean-up, logic addition to get orders to render WIP, not buying…

c5ea3df

…/selling with cost.

Added some basic order tests.

8445492

Removed unnecessary code from previous attempt at another thought pro…

03225f4

…cess.

Testing something to see if I can get it to buy with a price, know wh…

dbaba17

…at I need to do will add asap.

CheezItMan reviewed Dec 18, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stef -- Carets#32

Stef -- Carets#32
SesameSeeds wants to merge 20 commits intoAda-C8:masterfrom
SesameSeeds:master

SesameSeeds commented Dec 18, 2017

Uh oh!

SesameSeeds commented Dec 18, 2017 •

edited

Loading

Uh oh!

CheezItMan commented Dec 18, 2017

Uh oh!

CheezItMan Dec 18, 2017

Uh oh!

CheezItMan Dec 18, 2017

Uh oh!

CheezItMan Dec 18, 2017

Uh oh!

CheezItMan Dec 18, 2017

Uh oh!

CheezItMan Dec 18, 2017

Uh oh!

CheezItMan Dec 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

SesameSeeds commented Dec 18, 2017

Ada Trader

Comprehension Questions

Uh oh!

SesameSeeds commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CheezItMan commented Dec 18, 2017

Ada Trader

What We're Looking For

Uh oh!

CheezItMan Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

CheezItMan Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

CheezItMan Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

CheezItMan Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

CheezItMan Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

CheezItMan Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SesameSeeds commented Dec 18, 2017 •

edited

Loading